RDKB-58910 : Move the WAN IPV6 configuration from LAN bridge#79
RDKB-58910 : Move the WAN IPV6 configuration from LAN bridge#79S-Parthiban-Selvaraj wants to merge 81 commits intomainfrom
Conversation
Reason for change: Solving Build errors
Test Procedure:
Updated in Jira.
Risks: none
Priority: P1
Signed-off-by: parthiban.selvaraj <parthiban.selvaraj@sky.uk>
Signed-off-by: parthiban.selvaraj <parthiban.selvaraj@sky.uk>
Signed-off-by: parthiban.selvaraj <parthiban.selvaraj@sky.uk>
Signed-off-by: Parthiban Selvaraj <parthiban.selvaraj@sky.uk>
Signed-off-by: Parthiban Selvaraj <parthiban.selvaraj@sky.uk>
Signed-off-by: S-Parthiban-Selvaraj <parthiban.selvaraj@sky.uk>
Signed-off-by: S-Parthiban-Selvaraj <parthiban.selvaraj@sky.uk>
Signed-off-by: S-Parthiban-Selvaraj <parthiban.selvaraj@sky.uk>
Signed-off-by: S-Parthiban-Selvaraj <parthiban.selvaraj@sky.uk>
Signed-off-by: Parthiban Selvaraj <parthiban.selvaraj@sky.uk>
Signed-off-by: S-Parthiban-Selvaraj <parthiban.selvaraj@sky.uk>
Signed-off-by: Parthiban Selvaraj <parthiban.selvaraj@sky.uk>
Signed-off-by: S-Parthiban-Selvaraj <parthiban.selvaraj@sky.uk>
Signed-off-by: S-Parthiban-Selvaraj <parthiban.selvaraj@sky.uk>
Signed-off-by: S-Parthiban-Selvaraj <parthiban.selvaraj@sky.uk>
Signed-off-by: Parthiban Selvaraj <parthiban.selvaraj@sky.uk>
Signed-off-by: S-Parthiban-Selvaraj <parthiban.selvaraj@sky.uk>
…eint after WFO Signed-off-by: Parthiban Selvaraj <parthiban.selvaraj@sky.uk>
Signed-off-by: Parthiban Selvaraj <parthiban.selvaraj@sky.uk>
Fixing Virtual interface name set. Signed-off-by: S-Parthiban-Selvaraj <parthiban.selvaraj@sky.uk>
Signed-off-by: Parthiban Selvaraj <parthiban.selvaraj@sky.uk>
Signed-off-by: Parthiban Selvaraj <parthiban.selvaraj@sky.uk>
…ive VISM for products use the same default name for all interfaces Signed-off-by: Parthiban Selvaraj <parthiban.selvaraj@sky.uk>
Signed-off-by: S-Parthiban-Selvaraj <parthiban.selvaraj@sky.uk>
Signed-off-by: S-Parthiban-Selvaraj <parthiban.selvaraj@sky.uk>
Signed-off-by: Parthiban Selvaraj <parthiban.selvaraj@sky.uk>
Signed-off-by: Parthiban Selvaraj <parthiban.selvaraj@sky.uk>
Signed-off-by: Parthiban Selvaraj <parthiban.selvaraj@sky.uk>
New Tags: https://github.com/rdkcentral/RdkWanManager/releases/tag/v2.7.0 What's Changed RDKCOM-5173 REFPLTB-3128: [TDK][AUTO][RPI4][CELLULAR MANAGER] Switching back to ethwan from cellular mode not working as expected by @ksaipr036 RDKB-57254 : MAP-T Unification by @S-Parthiban-Selvaraj in Signed-off-by: Parthiban Selvaraj <parthiban.selvaraj@sky.uk>
There was a problem hiding this comment.
Pull Request Overview
This PR moves WAN IPv6 configuration from the LAN bridge to the WAN interface itself, addressing an issue where IANA is not assigned by the BNG (Broadband Network Gateway). The changes enable the creation of IPv6 addresses on WAN interfaces from delegated prefixes when IANA addresses are not provided.
Key changes:
- Removed IPv6 configuration from LAN bridge and moved it to WAN interfaces
- Added new function to construct WAN IPv6 addresses from IAPD (IA Prefix Delegation)
- Updated IPv6 utility functions to work with virtual interfaces instead of interface names
- Cleaned up LAN-specific IPv6 handling code and legacy bridge mode logic
Reviewed Changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| wanmgr_sysevents.h | Removed LAN sync support structures and duplicate sysevent registrations |
| wanmgr_sysevents.c | Cleaned up LAN state variables and duplicate sysevent handling |
| wanmgr_net_utils.h/c | Refactored IPv6 utility function to use virtual interface pointer |
| wanmgr_ipc.h/c | Updated IHC messaging to use virtual interface structure |
| wanmgr_interface_sm.c | Major refactoring of IPv6 address handling and state management |
| wanmgr_dhcpv6_apis.h/c | Added new function to construct WAN addresses from IAPD |
| wanmgr_dhcp_event_handler.c | Updated to use new IPv6 utility functions |
| dhcpv6c_msg_apis.c | Added validation for prefix assignment |
| wanmgr_rdkbus_common.h | Removed obsolete bridge interface definition |
Comments suppressed due to low confidence (1)
source/WanManager/wanmgr_dhcpv6_apis.c:1
- This condition appears incorrect. It checks for neither address nor prefix assigned, but the comment suggests it should handle the case where only IAPD is received. The condition should be
!pDhcpv6Data->addrAssigned && pDhcpv6Data->prefixAssigned.
/*
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Signed-off-by: Parthiban Selvaraj <parthiban.selvaraj@sky.uk>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 8 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
source/WanManager/wanmgr_sysevents.c
Outdated
| sysevent_set_options(sysevent_msg_fd, sysevent_msg_token, SYSEVENT_IPV6_TOGGLE, TUPLE_FLAG_EVENT); | ||
| sysevent_setnotification(sysevent_msg_fd, sysevent_msg_token, SYSEVENT_IPV6_TOGGLE, &default_route_change_event_asyncid); |
There was a problem hiding this comment.
Duplicate sysevent registration for SYSEVENT_IPV6_TOGGLE. Lines 620-621 are identical to lines 623-624, causing the same event to be registered twice with the same parameters.
| sysevent_set_options(sysevent_msg_fd, sysevent_msg_token, SYSEVENT_IPV6_TOGGLE, TUPLE_FLAG_EVENT); | |
| sysevent_setnotification(sysevent_msg_fd, sysevent_msg_token, SYSEVENT_IPV6_TOGGLE, &default_route_change_event_asyncid); |
| if(dad_flag == 0 || route_flag == 0) | ||
| { | ||
| CcspTraceError(("%s %d dad_flag[%d] route_flag[%d] Failed \n", __FUNCTION__, __LINE__,dad_flag,route_flag)); | ||
| CcspTraceError(("%s %d dad_flag[%s] route_flag[%s] Failed \n", __FUNCTION__, __LINE__,dad_flag ?"SUCCESS":"FAILED",route_flag?"SUCCESS":"FAILED")); |
There was a problem hiding this comment.
Format string expects integer values (%d) but provides strings. The format specifiers should be %s to match the ternary expressions that output strings.
| { | ||
| /* In an IPv6 lease, if only IAPD is received and we never received IANA, | ||
| * We can use the received IAPD to construct a Ipv6 /128 address which can be used for managerment and voice ... | ||
| * If we reach this point, only IAPD has been received. Canculate Wan Ipv6 address |
There was a problem hiding this comment.
Corrected spelling of 'Canculate' to 'Calculate'.
| * If we reach this point, only IAPD has been received. Canculate Wan Ipv6 address | |
| * If we reach this point, only IAPD has been received. Calculate Wan Ipv6 address |
| pDhcpv6Data->prefixAssigned = leaseInfo->prefixAssigned; | ||
| pDhcpv6Data->domainNameAssigned = leaseInfo->domainNameAssigned; | ||
| pDhcpv6Data->ipv6_TimeOffset = leaseInfo->ipv6_TimeOffset; | ||
| if(!pDhcpv6Data->addrAssigned && !pDhcpv6Data->prefixAssigned) |
There was a problem hiding this comment.
Logic error: the condition checks if both addrAssigned and prefixAssigned are false, but according to the comment and context, the intention is to construct a WAN address when IANA is not assigned but IAPD is received. The condition should be if(!pDhcpv6Data->addrAssigned && pDhcpv6Data->prefixAssigned) to match the stated behavior.
| if(!pDhcpv6Data->addrAssigned && !pDhcpv6Data->prefixAssigned) | |
| if(!pDhcpv6Data->addrAssigned && pDhcpv6Data->prefixAssigned) |
| WanMgr_Rbus_EventPublishHandler(param_name, "", RBUS_STRING); | ||
| WanManager_UpdateInterfaceStatus (p_VirtIf, WANMGR_IFACE_CONNECTION_IPV6_DOWN); | ||
|
|
||
| //Disable accept_ra |
There was a problem hiding this comment.
Comment lacks proper spacing. Should be '// Disable accept_ra' with space after '//'.
| //Disable accept_ra | |
| // Disable accept_ra |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * In this scenario, we should not consider IANA or IAPD as deleted. | ||
| * If we reach this point, only IAPD has been renewed. Use the previous IANA details. */ | ||
| * If we reach this point, only IAPD has been renewed. Use the previous IANA details. | ||
| */ |
There was a problem hiding this comment.
Corrected spelling of 'Canculate' to 'Calculate'.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 12 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| snprintf(cmdLine, sizeof(cmdLine), "ip -6 route flush %s ", prefix); | ||
| #endif | ||
| if (WanManager_DoSystemActionWithStatus("ip -6 route flush PREFIX ", cmdLine) != 0) | ||
| snprintf(cmdLine, sizeof(cmdLine), "ip -6 route flush match %s ", p_VirtIf->IP.Ipv6Data.address); |
There was a problem hiding this comment.
In DEL_ADDR, ip -6 route flush match is run with the interface IPv6 host address (...match <addr>). That will only flush routes whose destination matches that /128, and will not remove routes for the delegated prefix (e.g., the ip -6 route add <sitePrefix> dev brlan0 route added during PD handling). Please flush based on the delegated prefix (or remove the specific prefix route you added) rather than matching the /128 address.
| snprintf(cmdLine, sizeof(cmdLine), "ip -6 route flush match %s ", p_VirtIf->IP.Ipv6Data.address); | |
| snprintf(cmdLine, sizeof(cmdLine), "ip -6 route flush dev %s", p_VirtIf->Name); |
| @@ -444,25 +444,7 @@ ANSC_STATUS WanMgr_SendMsgToIHC (ipoe_msg_type_t msgType, char *ifName) | |||
| if (msgType == IPOE_MSG_WAN_CONNECTION_IPV6_UP) | |||
| { | |||
| // V6 UP Message needs Wan V6 IP | |||
| char* pattern = NULL; | |||
| char ipv6_prefix[INET6_ADDRSTRLEN] = {0}; | |||
|
|
|||
| sysevent_get(sysevent_fd, sysevent_token, SYSCFG_FIELD_IPV6_PREFIX, ipv6_prefix, sizeof(ipv6_prefix)); | |||
| if(ipv6_prefix == NULL || *ipv6_prefix == '\0'|| (0 == strncmp(ipv6_prefix, "(null)", strlen("(null)")))) | |||
| { | |||
| CcspTraceError(("[%s-%d] Unable to get ipv6_prefix..\n", __FUNCTION__, __LINE__)); | |||
| return ANSC_STATUS_FAILURE; | |||
| } | |||
|
|
|||
| pattern = strstr(ipv6_prefix, "/"); | |||
| if (pattern == NULL) | |||
| { | |||
| CcspTraceError(("[%s-%d] Invalid ipv6_prefix :%s\n", __FUNCTION__, __LINE__, ipv6_prefix)); | |||
| return ANSC_STATUS_FAILURE; | |||
| } | |||
| sprintf(pattern, "%c%c", '1', '\0'); //Form the global address with ::1 | |||
| strncpy(msgBody.ipv6Address, ipv6_prefix, sizeof(ipv6_prefix)); | |||
|
|
|||
| strncpy(msgBody.ipv6Address, p_VirtIf->IP.Ipv6Data.address, sizeof(msgBody.ipv6Address) - 1); | |||
| if( 0 == syscfg_get( NULL, "ntp_server1", domainName, sizeof(domainName)) ) | |||
| { | |||
| WanMgr_RemoveSingleQuote(domainName); | |||
| @@ -477,16 +459,7 @@ ANSC_STATUS WanMgr_SendMsgToIHC (ipoe_msg_type_t msgType, char *ifName) | |||
| } | |||
| else if (msgType == IPOE_MSG_WAN_CONNECTION_UP) | |||
| { | |||
| char ipv4_wan_address[IP_ADDR_LENGTH] = {0}; | |||
| char sysevent_param_name[BUFLEN_64] = {0}; | |||
| snprintf(sysevent_param_name, sizeof(sysevent_param_name), SYSEVENT_IPV4_IP_ADDRESS, ifName); | |||
| sysevent_get(sysevent_fd, sysevent_token, sysevent_param_name, ipv4_wan_address, sizeof(ipv4_wan_address)); | |||
| if(ipv4_wan_address == NULL || *ipv4_wan_address == '\0'|| (0 == strncmp(ipv4_wan_address, "(null)", strlen("(null)")))) | |||
| { | |||
| CcspTraceError(("[%s-%d] Unable to get ipv4_erouter0_ipaddr..\n", __FUNCTION__, __LINE__)); | |||
| return ANSC_STATUS_FAILURE; | |||
| } | |||
| strncpy(msgBody.ipv4Address, ipv4_wan_address, sizeof(ipv4_wan_address)); | |||
| strncpy(msgBody.ipv4Address, p_VirtIf->IP.Ipv4Data.ip, sizeof(msgBody.ipv4Address)-1); | |||
|
|
|||
| if( 0 == syscfg_get( NULL, "ntp_server1", domainName, sizeof(domainName)) ) | |||
| { | |||
| @@ -500,9 +473,9 @@ ANSC_STATUS WanMgr_SendMsgToIHC (ipoe_msg_type_t msgType, char *ifName) | |||
|
|
|||
| CcspTraceInfo(("[%s-%d] Sending IPOE_MSG_WAN_CONNECTION_UP msg with addr :%s and domainName: [%s] \n", __FUNCTION__, __LINE__, msgBody.ipv4Address, msgBody.domainName)); | |||
| } | |||
| strncpy(msgBody.ifName, ifName, IFNAME_LENGTH-1); | |||
| strncpy(msgBody.ifName, p_VirtIf->Name, IFNAME_LENGTH-1); | |||
|
|
|||
| CcspTraceInfo(("[%s-%d] Sending msg = %d for interface %s \n", __FUNCTION__, __LINE__, msgType, ifName)); | |||
| CcspTraceInfo(("[%s-%d] Sending msg = %d for interface %s \n", __FUNCTION__, __LINE__, msgType, p_VirtIf->Name)); | |||
There was a problem hiding this comment.
WanMgr_SendMsgToIHC now dereferences p_VirtIf unconditionally and no longer validates that the IPv4/IPv6 address fields are populated before sending (the previous implementation rejected empty/"(null)" values). Please add a null check for p_VirtIf and validate p_VirtIf->IP.Ipv4Data.ip / p_VirtIf->IP.Ipv6Data.address for the relevant message types to avoid sending empty addresses (or crashing if p_VirtIf is unexpectedly null).
| /** | ||
| * @brief Sets up the IPv6 /128 address for the LAN bridge. | ||
| * This function creates an IPv6 /128 address for the LAN bridge, | ||
| * configures all necessary system events for the IPv6 LAN, | ||
| * and restarts the DHCPv6 server and RA server if required. | ||
| * @param pVirtIf Pointer to the virtual interface structure. | ||
| * @return int 0 on success, non-zero on failure. | ||
| */ |
There was a problem hiding this comment.
The new docstring for setUpLanPrefixIPv6 says it "Sets up the IPv6 /128 address for the LAN bridge" and that it "creates" the address, but the implementation no longer assigns an IPv6 address to brlan0 (it only sets sysevents and adds a route). Please update the comment to match the actual behavior (or restore the missing address assignment if it is still required).
| { | ||
| char newPref[128] = {0}; | ||
| inet_ntop(AF_INET6, &prefix, newPref, sizeof(newPref)); | ||
| CcspTraceInfo(("%s %d EUI64 format is enabled using new prefix %s \n", __FUNCTION__, __LINE__, newPref)); | ||
| snprintf(cmdLine, sizeof(cmdLine), "%s/%d", newPref, prefix_length); | ||
| WanMgr_create_eui64_ipv6_address(cmdLine, pIpv6DataNew->ifname, pIpv6DataNew->address); |
There was a problem hiding this comment.
wanmgr_construct_wan_address_from_IAPD calls WanMgr_create_eui64_ipv6_address, but that helper is only compiled when FEATURE_RDKB_CONFIGURABLE_WAN_INTERFACE is defined (it is static inside that #if). Since wanmgr_construct_wan_address_from_IAPD is compiled unconditionally, builds that don't define FEATURE_RDKB_CONFIGURABLE_WAN_INTERFACE will fail (undefined symbol / implicit declaration). Please either move wanmgr_construct_wan_address_from_IAPD under the same feature guard, or provide a non-conditional implementation/declaration for the EUI-64 helper (or a fallback path).
| { | |
| char newPref[128] = {0}; | |
| inet_ntop(AF_INET6, &prefix, newPref, sizeof(newPref)); | |
| CcspTraceInfo(("%s %d EUI64 format is enabled using new prefix %s \n", __FUNCTION__, __LINE__, newPref)); | |
| snprintf(cmdLine, sizeof(cmdLine), "%s/%d", newPref, prefix_length); | |
| WanMgr_create_eui64_ipv6_address(cmdLine, pIpv6DataNew->ifname, pIpv6DataNew->address); | |
| { | |
| #ifdef FEATURE_RDKB_CONFIGURABLE_WAN_INTERFACE | |
| char newPref[128] = {0}; | |
| inet_ntop(AF_INET6, &prefix, newPref, sizeof(newPref)); | |
| CcspTraceInfo(("%s %d EUI64 format is enabled using new prefix %s \n", __FUNCTION__, __LINE__, newPref)); | |
| snprintf(cmdLine, sizeof(cmdLine), "%s/%d", newPref, prefix_length); | |
| WanMgr_create_eui64_ipv6_address(cmdLine, pIpv6DataNew->ifname, pIpv6DataNew->address); | |
| #else | |
| CcspTraceInfo(("%s %d EUI64 format is enabled in configuration but FEATURE_RDKB_CONFIGURABLE_WAN_INTERFACE is not defined; using WAN SUFFIX %d instead \n", | |
| __FUNCTION__, __LINE__, WAN_SUFFIX)); | |
| prefix.s6_addr[15] = WAN_SUFFIX; // Setting the last byte for WAN address | |
| inet_ntop(AF_INET6, &prefix, pIpv6DataNew->address, sizeof(pIpv6DataNew->address)); | |
| #endif |
| char cmdLine[128] = {0}; | ||
| char prefix[BUFLEN_48] = {0}; | ||
| char prefixAddr[BUFLEN_48] = {0}; | ||
| char IfaceName[BUFLEN_16] = {0}; | ||
| int BridgeMode = 0; | ||
|
|
||
| memset(prefix, 0, sizeof(prefix)); | ||
| sysevent_get(sysevent_fd, sysevent_token, SYSEVENT_FIELD_IPV6_PREFIX, prefix, sizeof(prefix)); | ||
|
|
||
| memset(prefixAddr, 0, sizeof(prefixAddr)); | ||
| sysevent_get(sysevent_fd, sysevent_token, SYSEVENT_GLOBAL_IPV6_PREFIX_SET, prefixAddr, sizeof(prefixAddr)); | ||
|
|
||
| { //TODO : temporary debug code to identify the bridgemode sysevent failure issue. | ||
| char Output[BUFLEN_16] = {0}; | ||
| if (sysevent_get(sysevent_fd, sysevent_token, "bridge_mode", Output, sizeof(Output)) !=0) | ||
| { | ||
| CcspTraceError(("%s-%d: bridge_mode sysevent get failed. \n", __FUNCTION__, __LINE__)); | ||
| } | ||
| BridgeMode = atoi(Output); | ||
| CcspTraceInfo(("%s-%d: <<DEBUG>> bridge_mode sysevent value set to =%d \n", __FUNCTION__, __LINE__, BridgeMode)); | ||
| } | ||
|
|
||
| /*TODO: | ||
| *Below Code should be removed once V6 Prefix/IP is assigned on erouter0 Instead of brlan0 for sky Devices. | ||
| */ | ||
| strcpy(IfaceName, LAN_BRIDGE_NAME); | ||
| if (WanMgr_isBridgeModeEnabled() == TRUE) | ||
| { | ||
| memset(IfaceName, 0, sizeof(IfaceName)); | ||
| strncpy(IfaceName, ifname, strlen(ifname)); | ||
| } | ||
|
|
||
| CcspTraceInfo(("%s-%d: IfaceName=%s \n", __FUNCTION__, __LINE__, IfaceName)); | ||
|
|
||
| switch (opr) | ||
| { | ||
| case DEL_ADDR: | ||
| { | ||
| if (strlen(prefix) > 0) | ||
| if (strlen( p_VirtIf->IP.Ipv6Data.address) > 0) | ||
| { | ||
| #if !(defined (_XB6_PRODUCT_REQ_) || defined (_CBR2_PRODUCT_REQ_) || defined(_PLATFORM_RASPBERRYPI_)) || defined(_RDKB_GLOBAL_PRODUCT_REQ_) //Do not delete prefix from LAn bridge for the comcast platforms. | ||
| #if defined(_RDKB_GLOBAL_PRODUCT_REQ_) | ||
| WanMgr_Config_Data_t *pWanConfigData = WanMgr_GetConfigData_locked(); | ||
| unsigned char ConfigureWANIPv6OnLANBridgeSupport = FALSE; | ||
|
|
||
| if( NULL != pWanConfigData ) | ||
| { | ||
| ConfigureWANIPv6OnLANBridgeSupport = pWanConfigData->data.ConfigureWANIPv6OnLANBridgeSupport; | ||
| WanMgrDml_GetConfigData_release(pWanConfigData); | ||
| } | ||
| memset(cmdLine, 0, sizeof(cmdLine)); | ||
| snprintf(cmdLine, sizeof(cmdLine), "ip -6 addr del %s/128 dev %s", p_VirtIf->IP.Ipv6Data.address, p_VirtIf->Name); | ||
| if (WanManager_DoSystemActionWithStatus("ip -6 addr del ADDR dev xxxx", cmdLine) != 0) | ||
| CcspTraceError(("failed to run cmd: %s", cmdLine)); | ||
|
|
||
| if ( TRUE == ConfigureWANIPv6OnLANBridgeSupport ) | ||
| #endif /** _RDKB_GLOBAL_PRODUCT_REQ_ */ | ||
| { | ||
| memset(cmdLine, 0, sizeof(cmdLine)); | ||
| snprintf(cmdLine, sizeof(cmdLine), "ip -6 addr del %s/64 dev %s", prefixAddr, IfaceName); | ||
| if (WanManager_DoSystemActionWithStatus("ip -6 addr del ADDR dev xxxx", cmdLine) != 0) | ||
| CcspTraceError(("failed to run cmd: %s", cmdLine)); | ||
| } | ||
| #endif | ||
| memset(cmdLine, 0, sizeof(cmdLine)); | ||
| #if defined(FEATURE_RDKB_CONFIGURABLE_WAN_INTERFACE) | ||
| snprintf(cmdLine, sizeof(cmdLine), "ip -6 route flush match %s ", prefix); | ||
| #else | ||
| snprintf(cmdLine, sizeof(cmdLine), "ip -6 route flush %s ", prefix); | ||
| #endif | ||
| if (WanManager_DoSystemActionWithStatus("ip -6 route flush PREFIX ", cmdLine) != 0) | ||
| snprintf(cmdLine, sizeof(cmdLine), "ip -6 route flush match %s ", p_VirtIf->IP.Ipv6Data.address); | ||
|
|
There was a problem hiding this comment.
cmdLine is only 128 bytes, but the new commands include an IPv6 literal (up to ~45 chars) plus an interface name up to BUFLEN_64 (DML_VIRTUAL_IFACE.Name), which can exceed 128 and get truncated by snprintf, causing the system action to fail. Please increase the buffer (e.g., BUFLEN_256) or compute the required length to ensure the full command is preserved.
| if(IPv6EUI64FormatSupport) | ||
| { | ||
| char newPref[128] = {0}; | ||
| inet_ntop(AF_INET6, &prefix, newPref, sizeof(newPref)); | ||
| CcspTraceInfo(("%s %d EUI64 format is enabled using new prefix %s \n", __FUNCTION__, __LINE__, newPref)); | ||
| snprintf(cmdLine, sizeof(cmdLine), "%s/%d", newPref, prefix_length); | ||
| WanMgr_create_eui64_ipv6_address(cmdLine, pIpv6DataNew->ifname, pIpv6DataNew->address); | ||
| } | ||
| else | ||
| { | ||
| CcspTraceInfo(("%s %d EUI64 format is not enabled using WAN SUFFIX %d \n", __FUNCTION__, __LINE__, WAN_SUFFIX)); | ||
| prefix.s6_addr[15] = WAN_SUFFIX; // Setting the last byte for WAN address | ||
| inet_ntop(AF_INET6, &prefix, pIpv6DataNew->address, sizeof(pIpv6DataNew->address)); | ||
| } | ||
|
|
||
| pIpv6DataNew->addrAssigned = true; | ||
| pIpv6DataNew->addrCmd = IFADDRCONF_ADD; | ||
|
|
||
| CcspTraceInfo(("%s %d Calculated WAN network IP %s/128 \n", __FUNCTION__, __LINE__, pIpv6DataNew->address)); | ||
| //Since this address calculated by us, it will be assigned by the DHCPv6c client. Assign the address on the Wan interface | ||
| memset(cmdLine, 0, sizeof(cmdLine)); | ||
| snprintf(cmdLine, sizeof(cmdLine), "ip -6 addr add %s/128 dev %s", pIpv6DataNew->address, pIpv6DataNew->ifname); | ||
| if (WanManager_DoSystemActionWithStatus(__FUNCTION__, cmdLine) != 0) | ||
| CcspTraceError(("failed to run cmd: %s", cmdLine)); | ||
|
|
||
| memset(cmdLine, 0, sizeof(cmdLine)); | ||
| snprintf(cmdLine, sizeof(cmdLine), COSA_DML_WANIface_ADDR_SYSEVENT_NAME , pIpv6DataNew->ifname); | ||
| sysevent_set(sysevent_fd, sysevent_token, cmdLine, pIpv6DataNew->address, 0); | ||
|
|
||
| return 0; |
There was a problem hiding this comment.
wanmgr_construct_wan_address_from_IAPD sets addrAssigned=true and returns 0 regardless of whether address generation or ip -6 addr add actually succeeded (return value from WanMgr_create_eui64_ipv6_address is ignored; WanManager_DoSystemActionWithStatus failure is only logged). This can leave WanManager believing an IPv6 address exists when it does not. Please check these return codes and propagate failure (return -1 and avoid updating addrAssigned / sysevents on failure).
| // Convert prefix to binary format | ||
| if (inet_pton(AF_INET6, iapd_prefix, &prefix) != 1) | ||
| { | ||
| fprintf(stderr, "Invalid IPv6 prefix\n"); |
There was a problem hiding this comment.
wanmgr_construct_wan_address_from_IAPD writes directly to stderr (fprintf(stderr, ...)) on parse failure. This bypasses the component logging used elsewhere and may not be captured in production logs. Please use the existing CcspTraceError logging (and include the offending prefix) instead of writing to stderr.
| fprintf(stderr, "Invalid IPv6 prefix\n"); | |
| CcspTraceError(("%s %d Invalid IPv6 prefix: %s\n", __FUNCTION__, __LINE__, iapd_prefix)); |
| if(!pDhcpv6Data->addrAssigned && pDhcpv6Data->prefixAssigned) | ||
| { | ||
| /* In an IPv6 lease, if only IAPD is received and we never received IANA, | ||
| * We can use the received IAPD to construct a Ipv6 /128 address which can be used for managerment and voice ... | ||
| * If we reach this point, only IAPD has been received. Canculate Wan Ipv6 address | ||
| */ | ||
|
|
||
| CcspTraceInfo(("IANA is not assigned by DHCPV6. Constructing WAN address from the IAPD for Wan Interface \n")); | ||
| wanmgr_construct_wan_address_from_IAPD(pDhcpv6Data); | ||
| } | ||
|
|
There was a problem hiding this comment.
copyDhcpv6Data is a copy/helper routine but now has side effects (it may call wanmgr_construct_wan_address_from_IAPD, which runs ip -6 addr add and sets sysevents). Since DHCPv6 lease updates can arrive multiple times, this risks repeatedly trying to add the same /128 address (and doing potentially slow system calls while holding DhcpClientEvents_mutex). Please keep copyDhcpv6Data pure and move the WAN-address construction to the event-handling logic with proper idempotency (e.g., use ip -6 addr replace or check if the address is already present before adding).
| if(!pDhcpv6Data->addrAssigned && pDhcpv6Data->prefixAssigned) | |
| { | |
| /* In an IPv6 lease, if only IAPD is received and we never received IANA, | |
| * We can use the received IAPD to construct a Ipv6 /128 address which can be used for managerment and voice ... | |
| * If we reach this point, only IAPD has been received. Canculate Wan Ipv6 address | |
| */ | |
| CcspTraceInfo(("IANA is not assigned by DHCPV6. Constructing WAN address from the IAPD for Wan Interface \n")); | |
| wanmgr_construct_wan_address_from_IAPD(pDhcpv6Data); | |
| } |
| sysevent_set(sysevent_fd, sysevent_token, SYSEVENT_FIREWALL_RESTART, NULL, 0); | ||
|
|
||
| //RBUS_WAN_IP | ||
| //TODO : **************************** Check this ************************ |
There was a problem hiding this comment.
This placeholder TODO (//TODO : **************************** Check this ************************) is not actionable and will be hard to triage later. Please either remove it or replace it with a specific TODO that states what needs to be verified and under what conditions (ideally with a ticket reference).
| //TODO : **************************** Check this ************************ | |
| // TODO: Verify that the RBUS_WAN_IP IPv6 sysevent keys cleared below are correct for all product variants when WAN IPv6 goes down. |
| pDhcpv6Data->prefixAssigned = leaseInfo->prefixAssigned; | ||
| pDhcpv6Data->domainNameAssigned = leaseInfo->domainNameAssigned; | ||
| pDhcpv6Data->ipv6_TimeOffset = leaseInfo->ipv6_TimeOffset; | ||
| if(!pDhcpv6Data->addrAssigned && pDhcpv6Data->prefixAssigned) | ||
| { | ||
| /* In an IPv6 lease, if only IAPD is received and we never received IANA, | ||
| * We can use the received IAPD to construct a Ipv6 /128 address which can be used for managerment and voice ... | ||
| * If we reach this point, only IAPD has been received. Canculate Wan Ipv6 address | ||
| */ | ||
|
|
||
| CcspTraceInfo(("IANA is not assigned by DHCPV6. Constructing WAN address from the IAPD for Wan Interface \n")); | ||
| wanmgr_construct_wan_address_from_IAPD(pDhcpv6Data); | ||
| } |
There was a problem hiding this comment.
wanmgr_dhcp_event_handler.c calls wanmgr_construct_wan_address_from_IAPD() but does not include the header that declares it (wanmgr_dhcpv6_apis.h). This will rely on an implicit declaration in some toolchains (or fail to compile under C99/C11 with -Werror=implicit-function-declaration). Please include the proper header or add a forward declaration.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 10 comments.
Comments suppressed due to low confidence (1)
source/WanManager/wanmgr_ipc.c:438
WanMgr_SendMsgToIHCnow dereferencesp_VirtIfwithout validating it. Add a NULL check at the start of the function and fail early to avoid a crash if a caller passes NULL.
ANSC_STATUS WanMgr_SendMsgToIHC (ipoe_msg_type_t msgType, DML_VIRTUAL_IFACE *p_VirtIf )
{
int sock = -1;
int conn = -1;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * @brief Sets up the IPv6 /128 address for the LAN bridge. | ||
| * This function creates an IPv6 /128 address for the LAN bridge, | ||
| * configures all necessary system events for the IPv6 LAN, | ||
| * and restarts the DHCPv6 server and RA server if required. |
There was a problem hiding this comment.
The docstring says this function “sets up the IPv6 /128 address for the LAN bridge” and “creates an IPv6 /128 address”, but the current implementation no longer assigns an address on brlan0 (it only derives an address and sets sysevents). Update the comment to reflect the actual behavior (or restore the address assignment if it’s still required here).
| * @brief Sets up the IPv6 /128 address for the LAN bridge. | |
| * This function creates an IPv6 /128 address for the LAN bridge, | |
| * configures all necessary system events for the IPv6 LAN, | |
| * and restarts the DHCPv6 server and RA server if required. | |
| * @brief Sets up IPv6-related configuration for the LAN bridge. | |
| * This function derives the LAN IPv6 prefix from the delegated prefix, | |
| * configures the relevant system events and IPv6 settings needed for | |
| * LAN IPv6 operation, and allows other components (such as the DHCPv6 | |
| * and RA servers) to react to those changes as needed. |
| sysevent_set(sysevent_fd, sysevent_token, SYSEVENT_FIREWALL_RESTART, NULL, 0); | ||
|
|
||
| //RBUS_WAN_IP | ||
| //TODO : **************************** Check this ************************ |
There was a problem hiding this comment.
The added //TODO : **************************** Check this ************************ is a placeholder and doesn’t describe an actionable task. Please replace it with a specific TODO (what/why) and ideally a ticket reference, or remove it if not needed.
| //TODO : **************************** Check this ************************ |
| if (WanManager_DoSystemActionWithStatus(__FUNCTION__, cmdLine) != 0) | ||
| CcspTraceError(("failed to run cmd: %s", cmdLine)); |
There was a problem hiding this comment.
If ip -6 addr add ... fails, the function still continues and returns success. Propagate the failure (return -1) so callers don’t treat IPv6 as configured when it isn’t.
| if (WanManager_DoSystemActionWithStatus(__FUNCTION__, cmdLine) != 0) | |
| CcspTraceError(("failed to run cmd: %s", cmdLine)); | |
| int rc = WanManager_DoSystemActionWithStatus(__FUNCTION__, cmdLine); | |
| if (rc != 0) | |
| { | |
| CcspTraceError(("failed to run cmd: %s", cmdLine)); | |
| return -1; | |
| } |
| snprintf(cmdLine, sizeof(cmdLine), "ip -6 route flush match %s ", p_VirtIf->IP.Ipv6Data.address); | ||
|
|
||
| if (WanManager_DoSystemActionWithStatus(cmdLine, cmdLine) != 0) |
There was a problem hiding this comment.
ip -6 route flush match expects a prefix/CIDR. Flushing with p_VirtIf->IP.Ipv6Data.address (without /128) is likely a no-op/error and won’t remove routes for the delegated prefix. Use an appropriate CIDR (e.g., the delegated prefix in sitePrefix or %s/128 for the host route) and keep the first argument to WanManager_DoSystemActionWithStatus as a stable log tag rather than the full command string.
| snprintf(cmdLine, sizeof(cmdLine), "ip -6 route flush match %s ", p_VirtIf->IP.Ipv6Data.address); | |
| if (WanManager_DoSystemActionWithStatus(cmdLine, cmdLine) != 0) | |
| snprintf(cmdLine, sizeof(cmdLine), "ip -6 route flush match %s/128", p_VirtIf->IP.Ipv6Data.address); | |
| if (WanManager_DoSystemActionWithStatus("ip -6 route flush match ADDR/128", cmdLine) != 0) |
| if (msgType == IPOE_MSG_WAN_CONNECTION_IPV6_UP) | ||
| { | ||
| // V6 UP Message needs Wan V6 IP | ||
| char* pattern = NULL; | ||
| char ipv6_prefix[INET6_ADDRSTRLEN] = {0}; | ||
|
|
||
| sysevent_get(sysevent_fd, sysevent_token, SYSCFG_FIELD_IPV6_PREFIX, ipv6_prefix, sizeof(ipv6_prefix)); | ||
| if(ipv6_prefix == NULL || *ipv6_prefix == '\0'|| (0 == strncmp(ipv6_prefix, "(null)", strlen("(null)")))) | ||
| { | ||
| CcspTraceError(("[%s-%d] Unable to get ipv6_prefix..\n", __FUNCTION__, __LINE__)); | ||
| return ANSC_STATUS_FAILURE; | ||
| } | ||
|
|
||
| pattern = strstr(ipv6_prefix, "/"); | ||
| if (pattern == NULL) | ||
| { | ||
| CcspTraceError(("[%s-%d] Invalid ipv6_prefix :%s\n", __FUNCTION__, __LINE__, ipv6_prefix)); | ||
| return ANSC_STATUS_FAILURE; | ||
| } | ||
| sprintf(pattern, "%c%c", '1', '\0'); //Form the global address with ::1 | ||
| strncpy(msgBody.ipv6Address, ipv6_prefix, sizeof(ipv6_prefix)); | ||
|
|
||
| strncpy(msgBody.ipv6Address, p_VirtIf->IP.Ipv6Data.address, sizeof(msgBody.ipv6Address) - 1); |
There was a problem hiding this comment.
Before sending IPOE_MSG_WAN_CONNECTION_IPV6_UP, validate that p_VirtIf->IP.Ipv6Data.address is non-empty (and ideally a valid IPv6 string). Otherwise IHC can receive an “UP” message with an empty/invalid IPv6 address.
| return 0; | ||
| } |
There was a problem hiding this comment.
This helper is documented as returning -1 on failure, but it always returns 0 even when a system command fails. Propagate the command failure status (and return -1) so callers can react appropriately.
| wanmgr_construct_wan_address_from_IAPD(&Ipv6DataNew); | ||
| pNewIpcMsg->addrAssigned = true; |
There was a problem hiding this comment.
wanmgr_construct_wan_address_from_IAPD(&Ipv6DataNew) return value is ignored, but pNewIpcMsg->addrAssigned is forced true. If construction/assignment fails, IPv6 will be treated as up with no usable address. Gate addrAssigned updates on successful construction and handle the failure path.
| wanmgr_construct_wan_address_from_IAPD(&Ipv6DataNew); | |
| pNewIpcMsg->addrAssigned = true; | |
| /* Construct WAN IPv6 address from the delegated prefix only if possible. */ | |
| if (wanmgr_construct_wan_address_from_IAPD(&Ipv6DataNew) == 0) | |
| { | |
| Ipv6DataNew.addrAssigned = true; | |
| pNewIpcMsg->addrAssigned = true; | |
| } | |
| else | |
| { | |
| CcspTraceError(("%s %d Failed to construct WAN IPv6 address from IAPD. IPv6 address will not be marked as assigned.\n", | |
| __FUNCTION__, __LINE__)); | |
| } |
| else if (msgType == IPOE_MSG_WAN_CONNECTION_UP) | ||
| { | ||
| char ipv4_wan_address[IP_ADDR_LENGTH] = {0}; | ||
| char sysevent_param_name[BUFLEN_64] = {0}; | ||
| snprintf(sysevent_param_name, sizeof(sysevent_param_name), SYSEVENT_IPV4_IP_ADDRESS, ifName); | ||
| sysevent_get(sysevent_fd, sysevent_token, sysevent_param_name, ipv4_wan_address, sizeof(ipv4_wan_address)); | ||
| if(ipv4_wan_address == NULL || *ipv4_wan_address == '\0'|| (0 == strncmp(ipv4_wan_address, "(null)", strlen("(null)")))) | ||
| { | ||
| CcspTraceError(("[%s-%d] Unable to get ipv4_erouter0_ipaddr..\n", __FUNCTION__, __LINE__)); | ||
| return ANSC_STATUS_FAILURE; | ||
| } | ||
| strncpy(msgBody.ipv4Address, ipv4_wan_address, sizeof(ipv4_wan_address)); | ||
| strncpy(msgBody.ipv4Address, p_VirtIf->IP.Ipv4Data.ip, sizeof(msgBody.ipv4Address)-1); | ||
|
|
There was a problem hiding this comment.
Before sending IPOE_MSG_WAN_CONNECTION_UP, validate that p_VirtIf->IP.Ipv4Data.ip is non-empty (and ideally a valid IPv4 string). Otherwise IHC can receive an “UP” message with an empty/invalid IPv4 address.
| { | ||
| memset(cmdLine, 0, sizeof(cmdLine)); | ||
| snprintf(cmdLine, sizeof(cmdLine), "ip -6 addr change %s dev %s valid_lft %d preferred_lft %d ", prefixAddr, IfaceName, vallft, preflft); | ||
| snprintf(cmdLine, sizeof(cmdLine), "ip -6 addr change %s dev %s valid_lft %d preferred_lft %d ", p_VirtIf->IP.Ipv6Data.address, p_VirtIf->Name, p_VirtIf->IP.Ipv6Data.prefixVltime, p_VirtIf->IP.Ipv6Data.prefixPltime); |
There was a problem hiding this comment.
ip -6 addr change generally requires an address with prefix length (e.g., .../128). p_VirtIf->IP.Ipv6Data.address is stored without a prefix length, so this command will fail and lifetimes won’t be updated. Append the correct prefix length (likely /128) when building the command.
| snprintf(cmdLine, sizeof(cmdLine), "ip -6 addr change %s dev %s valid_lft %d preferred_lft %d ", p_VirtIf->IP.Ipv6Data.address, p_VirtIf->Name, p_VirtIf->IP.Ipv6Data.prefixVltime, p_VirtIf->IP.Ipv6Data.prefixPltime); | |
| snprintf(cmdLine, sizeof(cmdLine), "ip -6 addr change %s/128 dev %s valid_lft %d preferred_lft %d ", p_VirtIf->IP.Ipv6Data.address, p_VirtIf->Name, p_VirtIf->IP.Ipv6Data.prefixVltime, p_VirtIf->IP.Ipv6Data.prefixPltime); |
|
|
||
| prefix.s6_addr[7] += 0x01; // Use next subnet for WAN. First /64 will be used for LAN. | ||
|
|
There was a problem hiding this comment.
Incrementing prefix.s6_addr[7] to pick the “next /64” only works for some prefix lengths (e.g., /56). For prefixes like /57, /60, /48 this can alter provider-assigned prefix bits and generate an address outside the delegated range. Compute the next /64 by incrementing the subnet-id bits between prefix_length and 64 (bitwise), rather than hardcoding byte 7.
This PR moves WAN IPv6 configuration from the LAN bridge to the WAN interface itself, addressing an issue where IANA is not assigned by the BNG (Broadband Network Gateway). The changes enable the creation of IPv6 addresses on WAN interfaces from delegated prefixes when IANA addresses are not provided.
Key changes:
Removed IPv6 configuration from LAN bridge and moved it to WAN interfaces
Added new function to construct WAN IPv6 addresses from IAPD (IA Prefix Delegation)
Updated IPv6 utility functions to work with virtual interfaces instead of interface names
Cleaned up LAN-specific IPv6 handling code and legacy bridge mode logic
This PR is dependent on the following related PRs:
rdkcentral/telco-voice-manager#5
rdkcentral/utopia#69
#79
rdkcentral/provisioning-and-management#127